Skip to content

fix(transfer): stop quoting simple identifiers when migrating to GaussDB/OpenGauss - #6283

Open
q396921921 wants to merge 6 commits into
t8y2:mainfrom
q396921921:fix/data-transfer-strip-quotes
Open

fix(transfer): stop quoting simple identifiers when migrating to GaussDB/OpenGauss#6283
q396921921 wants to merge 6 commits into
t8y2:mainfrom
q396921921:fix/data-transfer-strip-quotes

Conversation

@q396921921

Copy link
Copy Markdown
Contributor

Problem

Data transfer (migration) always wrapped every target column/table name in
double quotes when the target was GaussDB or OpenGauss, locking in the
source's exact case. GaussDB can fold unquoted identifiers to a different
case than PostgreSQL does (e.g. its Oracle-compatible mode), so a later
unquoted query against the migrated table fails with "column does not
exist" even though the column is right there.

This matches the report in #6205: migrating a MySQL table with plain
lowercase field/table names to GaussDB produced a table whose fields
required quoting to query, which the user hadn't asked for and didn't
want.

Solution

quote_transfer_identifier (used by the data-transfer DDL/INSERT
generator) now only quotes an identifier when it actually needs it — mixed
case, special characters, or a reserved word — instead of quoting
unconditionally. This reuses the same heuristic (is_simple_lower_identifier

  • is_postgres_reserved_identifier) already used elsewhere in this codebase
    for GaussDB JDBC identifier quoting (quote_gaussdb_jdbc_identifier).

The change is deliberately scoped to GaussDB/OpenGauss only, not the wider
Postgres family (Postgres, Redshift, Kingbase, Highgo, Uxdb, Kwdb,
Vastbase). Those other targets have no reported case-folding issue, and
always-quoting them is harmless — narrowing the scope avoided touching ~30
existing tests that assert quoted output for plain Postgres transfer SQL,
which would have been a much larger, unrelated behavior change.

Testing

Added 3 regression tests in crates/dbx-core/src/transfer.rs, using the
exact columns from the issue's example table:

  • gaussdb_create_table_does_not_quote_simple_lowercase_identifiers — fails
    against the old code (reproduces the bug), passes after the fix.
  • gaussdb_create_table_still_quotes_identifiers_that_need_it — mixed-case
    and reserved-word identifiers still get quoted.
  • postgres_create_table_still_quotes_simple_lowercase_identifiers
    confirms plain Postgres behavior is unchanged.

Full transfer module test suite: 198/198 passing after rebasing onto
latest main.

Fixes #6205

@github-actions github-actions Bot added area/core Shared DBX core runtime bug Something isn't working labels Aug 14, 2026

@t8y2 t8y2 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

request changes: the quoting decision currently reuses the PostgreSQL reserved-word set for GaussDB/openGauss. These targets have their own keyword catalog; for example, the official GaussDB reserved-keyword table includes COMPACT. A simple lowercase identifier such as compact would therefore be emitted without quotes and can produce invalid target DDL.

Please base this decision on the target dialect keyword catalog and its unquoted-case rules, then add focused coverage for target-only reserved words and mixed-case identifiers. This database-semantic change also needs validation against a real GaussDB/openGauss instance before merge.

Official reference: https://support.huaweicloud.com/intl/en-us/distributed-devg-v8-gaussdb/gaussdb-12-0792.html

zhuxuesong added 3 commits August 15, 2026 11:39
…sDB/OpenGauss

Data transfer always wrapped every target column/table name in double
quotes for GaussDB/OpenGauss, locking in the source's exact case. GaussDB
can fold *unquoted* identifiers to a different case than Postgres does
(e.g. its Oracle-compatible mode), so a later unquoted query against the
migrated table fails with "column does not exist" even though the column
is there.

Only quote identifiers that actually need it (mixed case, special
characters, or a reserved word), reusing the same heuristic already used
for GaussDB JDBC identifier quoting. Scoped to GaussDB/OpenGauss only —
other Postgres-family targets (Postgres, Redshift, Kingbase, ...) have no
reported case-folding issue and always-quoting them is harmless.

Fixes t8y2#6205
The quoting decision for GaussDB/OpenGauss reused is_postgres_reserved_identifier,
but GaussDB reserves words PostgreSQL does not (e.g. COMPACT, per Huawei's
GaussDB(DWS) keyword reference). A plain lowercase column like `compact` passed
the "needs quoting" check and was emitted unquoted, producing invalid DDL on the
real target.

Add is_gaussdb_only_reserved_identifier with the GaussDB-only reserved words and
consult it alongside the Postgres list for Gaussdb/OpenGauss targets in both
quote_transfer_identifier and quote_gaussdb_jdbc_identifier (the latter also
serves plain Postgres connections, so it now takes database_type to keep the
extra reserved words scoped to GaussDB/OpenGauss only).

Addresses review feedback on t8y2#6283.
@q396921921
q396921921 force-pushed the fix/data-transfer-strip-quotes branch from faa82e0 to 68b185a Compare August 15, 2026 16:14
@q396921921

Copy link
Copy Markdown
Contributor Author

Pushed a fix for this. Summary of what changed and what's still open:

Keyword catalog gap — fixed. Checked the raw HTML of Huawei's official GaussDB(DWS) keyword reference (https://support.huaweicloud.com/intl/en-us/sqlreference-dws/dws_06_0007.html) directly rather than relying on a rendered/summarized view, and confirmed COMPACT is listed as Reserved (functions and types allowed) — exactly the case you flagged. Diffing that table against our existing Postgres reserved-word list turned up 24 more words GaussDB reserves that Postgres doesn't (AUTHID, BUCKETS, COMPACT, DELTAMERGE, FENCED, HDFSDIRECTORY, HOT, INTERNAL, LESS, MAXVALUE, MINUS, MODIFY, NLSSORT, PERFORMANCE, PLAN, PROCEDURE, RECYCLEBIN, REJECT, SYSDATE, TIMECAPSULE, TSTAG, TSTIME, TSFIELD, WARMUP).

Added is_gaussdb_only_reserved_identifier with that set and consulted it alongside the Postgres list, scoped to GaussDB/OpenGauss only, in both quoting call sites:

  • quote_transfer_identifier (the transfer DDL/INSERT path this PR touches)
  • quote_gaussdb_jdbc_identifier (pre-existing helper for live JDBC identifier quoting — it turned out to also serve plain Postgres connections, so it now takes database_type explicitly so the extra GaussDB words don't leak into real Postgres quoting decisions)

Added regression tests for target-only reserved words (compact, buckets, sysdate, minus, modify, plan, procedure) on both GaussDB and OpenGauss, plus a test confirming the same words are correctly not quoted on a real Postgres target — so the dialect split is exercised both ways, not just the GaussDB side.

Live-instance validation — not done, flagging honestly. I don't have access to a real GaussDB/openGauss instance. I tried spinning one up locally via docker pull --platform linux/amd64 opengauss/opengauss:5.0.0 (openGauss only ships amd64 images) on Apple Silicon, but the pull never completed in a reasonable time under emulation, so I couldn't get a live instance to execute DDL against.

One more caveat on the keyword source itself: the table I diffed against is Huawei's GaussDB(DWS) (the MPP/columnar variant) keyword reference — the core GaussDB/openGauss keyword doc pages are client-side rendered and I couldn't get a raw table out of them to cross-check directly. A few of the 24 added words (TSTAG, TSTIME, TSFIELD, HDFSDIRECTORY) are explicitly flagged in that same doc as used only by the hybrid-data-warehouse feature, so they may never actually appear as column names outside DWS — kept them in anyway since over-quoting is harmless (a strict superset of what's needed), same reasoning as the original PR's is_postgres_reserved_identifier reuse.

If you have access to a GaussDB or openGauss instance (or can point me at one), I'd appreciate either running the added test cases' DDL directly against it, or sharing connection details so I can validate myself before this merges.

@t8y2 t8y2 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes: the updated keyword fix still uses the GaussDB(DWS) keyword catalog as a hard-coded superset for core GaussDB/openGauss.

The PR's purpose is to stop quoting simple lowercase identifiers because quoting preserves case and can make later unquoted references fail. In that context, over-quoting is not harmless: if a DWS-only word is non-reserved on the actual core target, the new list recreates the original behavior for that identifier.

openGauss publishes target-specific reserved/non-reserved classifications and recommends consulting the actual keyword catalog (pg_get_keywords) rather than assuming another variant's list:

Please base the decision on the real target/version catalog or a verified exact core list, and validate the generated DDL on a writable GaussDB/openGauss instance in the relevant compatibility mode. Current unit tests pass, but all live GaussDB tests remain ignored.

@q396921921

Copy link
Copy Markdown
Contributor Author

I have a writable openGauss 5.0.0 instance available, so I ran the verification myself rather than asking you to find one.

Method: queried the instance's own pg_get_keywords() (the authoritative source both openGauss docs and I pointed to) for the 24 words this PR's is_gaussdb_only_reserved_identifier treats as GaussDB-only reserved, and cross-checked with an actual CREATE TABLE using each word unquoted.

Result: of the 24 words, 9 are not actually reserved on this real instance:

Word Real catcode on this instance
hot, nlssort, warmup not a keyword at all — absent from pg_get_keywords()
fenced, internal, plan, tsfield, tstag, tstime unreserved

The remaining 15 (authid, buckets, compact, deltamerge, hdfsdirectory, less, maxvalue, minus, modify, performance, procedure, recyclebin, reject, sysdate, timecapsule) are genuinely reserved (reserved or reserved (can be function or type name)), matching the DWS doc. I also confirmed end-to-end: an unquoted CREATE TABLE ... (compact int) fails with ERROR: syntax error at or near "compact" on this instance, confirming compact does need quoting.

Why this matters for the PR: this confirms the concern from the previous review — the DWS keyword catalog is not a safe superset for core GaussDB/openGauss. Right now is_gaussdb_only_reserved_identifier will force-quote fenced, internal, plan, tsfield, tstag, tstime, hot, nlssort, and warmup even though none of them need it on this instance — reintroducing the exact bug this PR sets out to fix (quoting locks in case, breaking later unquoted references) for any table using one of those as a column name.

Suggest trimming the set down to the 15 confirmed-reserved words, or better, deriving it from pg_get_keywords()/SELECT word FROM pg_get_keywords() WHERE catcode <> 'U' at connect time (or a version-gated static list sourced from that catalog) rather than a hand-diffed DWS doc snapshot.

Happy to share the exact test queries I used if useful.

The GaussDB-only reserved-word list in is_gaussdb_only_reserved_identifier
was diffed from Huawei's GaussDB(DWS) keyword reference, but DWS is the
MPP/columnar variant and isn't a reliable proxy for core GaussDB/openGauss.

Cross-checked all 24 words against a writable openGauss 5.0.0 instance's
own pg_get_keywords(): 9 of them are not actually reserved on the core
engine (hot/nlssort/warmup aren't keywords at all; fenced/internal/plan/
tsfield/tstag/tstime are unreserved). Quoting them would have reintroduced
the case-locking bug this PR fixes. Dropped those 9 and kept the remaining
15 confirmed reserved/reserved(can be function or type name) words;
compact was additionally confirmed by running an unquoted CREATE TABLE
against that instance, which fails with a syntax error.

Extended the regression test to cover all 15 confirmed words plus a
negative check for the 9 dropped ones.

Addresses review feedback on t8y2#6283.
@q396921921

Copy link
Copy Markdown
Contributor Author

Pushed a fix acting on the instance-verification results from my last comment.

is_gaussdb_only_reserved_identifier is trimmed from 24 to the 15 words confirmed reserved on that real openGauss 5.0.0 instance (authid, buckets, compact, deltamerge, hdfsdirectory, less, maxvalue, minus, modify, performance, procedure, recyclebin, reject, sysdate, timecapsule). Dropped the 9 words that instance's pg_get_keywords() showed are not actually reserved (hot/nlssort/warmup aren't keywords at all; fenced/internal/plan/tsfield/tstag/tstime are unreserved) — keeping them would have reintroduced this PR's original bug (over-quoting locks in case).

Updated the doc comment on is_gaussdb_only_reserved_identifier to point at the real-instance verification instead of the DWS doc diff, and extended quotes_gaussdb_only_reserved_words_not_shared_with_postgres to assert on all 15 confirmed words plus a negative check that the 9 dropped words are not quoted.

cargo test -p dbx-core --lib sql_dialect (100 tests), cargo fmt, and cargo clippy -p dbx-core --lib -- -D warnings all pass clean locally.

Live-instance DDL validation for this specific diff (as opposed to the standalone keyword checks I ran) is still outstanding — happy to run it if you can point me at an instance, otherwise let me know if the trimmed list itself needs anything else before merge.

@t8y2 t8y2 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new head still uses an incomplete static keyword subset, so valid migrations can generate unquoted reserved identifiers.

Compared with the openGauss 5.0 keyword catalog, the current PostgreSQL plus GaussDB list still misses at least csn, excluded, groupparent, nocycle, rownum, shrink, and verify. These names pass the simple-lowercase check and are emitted without quotes. A source column such as rownum can therefore make generated CREATE TABLE or INSERT fail on the target.

openGauss recommends checking the target keyword catalog through pg_get_keywords() and documents the target-specific classification:
https://docs.opengauss.org/en/docs/latest-lite/sql_reference/keywords.html

Please either query and cache the real target/version catalog once per connection or maintain separate, source-backed complete lists for GaussDB and OpenGauss. Add table, schema, column, CREATE TABLE, and INSERT tests for the missing words and validate the generated SQL on writable targets in the relevant compatibility modes.

…and-picked list

The GaussDB-only reserved-word list in is_gaussdb_only_reserved_identifier was
built by spot-checking words against a writable openGauss 5.0.0 instance one
at a time, which missed csn, excluded, groupparent, nocycle, rownum, shrink,
and verify — all reserved on that same instance but not in plain PostgreSQL,
so they were emitted unquoted and could break generated DDL/DML.

Diffed against the instance's full pg_get_keywords() result set (653 rows)
instead of individual words this time; the gap was exactly these 7, with no
further omissions. Extended the reserved-word test to cover them, and added
table/schema-name coverage to the CREATE TABLE regression test plus a new
INSERT regression test (previously only CREATE TABLE was covered).

Addresses review feedback on t8y2#6283.
@q396921921

Copy link
Copy Markdown
Contributor Author

Pushed a fix acting on the review feedback about the hand-picked list.

Instead of spot-checking individual words again, diffed is_gaussdb_only_reserved_identifier's existing 15-word list against the full pg_get_keywords() result set from the same writable openGauss 5.0.0 instance (653 rows) rather than checking words one at a time. Found 7 more genuinely reserved words the hand-picked list had missed: csn, excluded, groupparent, nocycle, rownum, shrink, verify — all confirmed reserved (R) or reserved, can be function or type name (T) on that instance.

Extended quotes_gaussdb_only_reserved_words_not_shared_with_postgres to cover all 22 words (and the negative Postgres check for the 3 newly added ones that also happen to look like plain words there). Also added table/schema-name coverage to the existing CREATE TABLE regression test (previously only checked column names), and a new gaussdb_insert_quotes_target_only_reserved_words test since the INSERT path shares quote_transfer_identifier but had no dedicated regression coverage of its own.

cargo test -p dbx-core --lib sql_dialect (100 tests) and cargo test -p dbx-core --lib transfer:: (201 tests) both pass clean locally.

Live-instance DDL/INSERT validation for this specific diff is still outstanding, same caveat as before.

@t8y2 t8y2 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes: the new catalog is complete for the tested openGauss 5.0.0 instance, but it is still applied statically to every GaussDB/openGauss version.

maxvalue is the concrete version-dependent counterexample. The official openGauss 5.0 source classifies it as RESERVED_KEYWORD, while current openGauss source classifies it as UNRESERVED_KEYWORD:

This PR permanently quotes maxvalue for all targets. On newer versions that reintroduces the original case-locking failure mode: a simple lowercase, non-reserved identifier is unnecessarily quoted and later unquoted references are folded differently.

Please make the keyword decision target/version aware—preferably by querying and caching pg_get_keywords() once per connection, or by selecting a source-backed catalog for the actual server version. Add regression coverage showing the differing maxvalue behavior across the supported versions and validate both paths on real targets.

…ware

The GaussDB-only reserved-word list used to decide when transfer output
needs quoting was a static list hand-diffed against one openGauss 5.0.0
instance. As the maintainer pointed out in review, that list is
version-dependent — `maxvalue` is reserved on openGauss 5.0 but not on
current/master openGauss, so permanently quoting it reintroduces the
original case-locking bug (t8y2#6205) on newer targets.

Replace the static list with a live `pg_get_keywords()` query, run once
per target connection and cached on `AppState` by pool_key
(`db::postgres::gaussdb_reserved_keywords`,
`AppState::gaussdb_reserved_keywords`). The live catalog is authoritative
when available and replaces (never unions with) the static list, so a
word only reserved on older versions is correctly left unquoted on newer
ones; falls back to the static list when no live catalog is available.
Only a deterministic outcome (query ran and failed/came back empty, e.g.
insufficient privileges) is cached — a merely transient failure (checkout
error, timeout) is retried on the next table rather than pinned to
"unavailable" for the pool's lifetime.

Threaded through the CREATE TABLE DDL and INSERT/UPSERT write paths in
both the SQL-source (`transfer_table`) and MongoDB-source
(`transfer_mongodb_table`) transfer flows, including the Overwrite-mode
TRUNCATE/DELETE step (extracted into a shared `transfer_overwrite_clear_sql`
helper) — that statement was initially missed and quoted using the stale
static list while CREATE TABLE already used the live catalog, which would
have made the two disagree on the same table name.

Added regression tests modeling both a 5.0-shaped and a current-shaped
live keyword catalog for `maxvalue` across the DDL, INSERT, and TRUNCATE
paths. Also validated end-to-end against two real openGauss instances (a
5.0.0 instance and a 7.0.0-RC1 instance) — pg_get_keywords() output,
generated DDL, and actual CREATE TABLE + SELECT execution all matched
expectations on both versions.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions github-actions Bot added the db/postgres Database: PostgreSQL label Aug 20, 2026
@q396921921

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed catch on maxvalue — pushed a fix that makes the keyword decision version-aware, using the approach you suggested.

What changed

  • db::postgres::gaussdb_reserved_keywords runs SELECT word FROM pg_get_keywords() WHERE catcode IN ('R','T') against the target connection once, and AppState::gaussdb_reserved_keywords caches the result per pool_key. When available, this live, per-server catalog is authoritative and replaces (never unions with) the static hand-diffed list — so a word that's reserved on 5.0 but not on a newer version is quoted differently per target, instead of being permanently locked to the 5.0 snapshot. Falls back to the static list when no live catalog is available (e.g. insufficient privileges, an engine without pg_get_keywords()), matching the pre-existing behavior in that case.
  • Only a deterministic failure (the query ran and errored or came back empty) is cached — a transient one (connection checkout error, timeout) is retried on the next table rather than pinning the connection to "unavailable" for its whole lifetime.
  • Threaded through both the CREATE TABLE DDL and the INSERT/UPSERT write paths, for both the SQL-source and MongoDB-source transfer flows, including the Overwrite-mode TRUNCATE/DELETE step (this one I initially missed in my own self-review — it was still using the static list while CREATE TABLE next to it already used the live catalog, which would have made the two disagree on the same table name; fixed and covered by a regression test).

Regression coverage

Added tests that inject a synthetic 5.0-shaped keyword set (maxvalue present) and a current-shaped one (maxvalue absent) and assert the CREATE TABLE, INSERT, and TRUNCATE paths all quote maxvalue accordingly — plus a None/no-live-catalog case confirming the static-list fallback is unchanged.

Real-target validation

Ran this end-to-end against two real openGauss instances:

Ready for another look whenever you have a chance.

@t8y2 t8y2 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes: the live pg_get_keywords() catalog now fixes the concrete maxvalue version difference when probing succeeds, but two fallback issues remain.

  1. crates/dbx-core/src/db/postgres.rs:101 maps every catalog query error to NotSupported, and crates/dbx-core/src/connection.rs:3902 caches that result for the pool lifetime. A transient disconnect or I/O error can therefore permanently select the static fallback and reproduce incorrect quoting on newer openGauss versions. Please classify transient connection/I/O/cancellation failures as retryable and add a first-failure/second-success regression test.

  2. crates/dbx-core/src/sql_dialect/identifiers.rs:451 still combines a successful live catalog with the static PostgreSQL reserved-word table. Please make the complete live R/T catalog authoritative when it is available, or provide source-backed tests proving the static subset is valid for every supported GaussDB/openGauss target.

The branch is also currently conflicting with main; resolve it after the behavior is corrected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/core Shared DBX core runtime bug Something isn't working db/postgres Database: PostgreSQL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] 数据传输时,源表字段及表名带引号或者反引号时,迁移到目标库时字段及表名能否去掉引号及反引号

2 participants